Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

nixos/postgresql: extension based hardening relaxation #355010

Merged
merged 4 commits into from
Nov 16, 2024
Merged

nixos/postgresql: extension based hardening relaxation #355010

merged 4 commits into from
Nov 16, 2024

Conversation

mweinelt
Copy link
Member

So this is the initial idea I had about how to relax the hardening when certain plugins are used.

I fully expect the certain scenario to be incomplete wrt. MemoryDenyWriteExecute, if it does indeed us V8.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@mweinelt mweinelt mentioned this pull request Nov 10, 2024
Merged
13 tasks
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 10, 2024
@Ma27
Copy link
Member

Ma27 commented Nov 10, 2024

FTR as elaborated in the previous PR I also think that this is the correct approach.

@mweinelt mweinelt force-pushed the pg-plugin-relax branch 2 times, most recently from 5aa3d62 to c083150 Compare November 10, 2024 21:33
@mweinelt mweinelt marked this pull request as ready for review November 10, 2024 21:33
@mweinelt
Copy link
Member Author

Okay, I moved a few things around and I think that should do the trick.

@nix-owners nix-owners bot requested a review from thoughtpolice November 10, 2024 21:34
@mweinelt mweinelt force-pushed the pg-plugin-relax branch 2 times, most recently from 433de7c to 34b0cad Compare November 11, 2024 01:40
wolfgangwalther
wolfgangwalther reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach is good!

nixos/tests/postgresql/postgresql.nix Outdated Show resolved Hide resolved
pkgs/servers/sql/postgresql/generic.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 11, 2024
@mweinelt mweinelt changed the title nixos/postgresql: relax hardening for plv8 plugin nixos/postgresql: extension based hardening relaxation Nov 11, 2024
@mweinelt mweinelt added this to the 24.11 milestone Nov 11, 2024
Ma27
Ma27 reviewed Nov 13, 2024
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolfgangwalther the current patch seems fine, the name of a derivation doesn't fail to evaluate if meta.broken = true;:

❯ ~/Projects/nixpkgs master → nix-instantiate -A postgresqlJitPackages.plv8.name --eval
"plv8-3.2.3"
❯ ~/Projects/nixpkgs master → nix-instantiate -A postgresqlJitPackages.plv8            
[...]
       error: Package ‘plv8-3.2.3’ in /home/ma27/Projects/nixpkgs/pkgs/servers/sql/postgresql/ext/plv8/default.nix:136 is marked as broken, refusing to evaluate.
[...]

@mweinelt can you rebase, please?

nixos/modules/services/databases/postgresql.nix Outdated Show resolved Hide resolved
@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented Nov 13, 2024 •
edited
Loading

the current patch seems fine, the name of a derivation doesn't fail to evaluate if meta.broken = true;:

I don't understand what that means. I might have missed something, but I think the current approach in here, i.e. adding plv8 to the postgresql test, will break this for the JIT variants. (After rebasing on my fix, ofc)

@Ma27
Copy link
Member

Ma27 commented Nov 13, 2024

@mweinelt OK rebased now to have a look. :)

I don't understand what that means. I might have missed something, but I think the current approach in here, i.e. adding plv8 to the postgresql test, will break this for the JIT variants. (After rebasing on my fix, ofc)

To me the conversation above read like the hardening checks in the module itself were the issue, however these can be evaluated even if the extensions are marked as broken.

But you're right, using plv8 in the test-case is the problem.

We already have 120 instances of PostgreSQL VM tests, I'd be reluctant to create five more.

I'd be fine with doing the plv8 check only in the non-JIT test-cases of the postgres test for now to get this out.
But I'd like to split this off to make sure we also have reasonable coverage of MemoryDenyWriteExecute being turned on.

An idea I just had was that we could actually build a single VM test for all extensions that require testing using the VM-test framework, but that'd obviously have the downside that postgresqlPackages.X.tests would fail if an unrelated extension is broken.

Idk. personally I'd be fine with the amount of VM tests, it's not like I'd expect contributors to run all of them.

@wolfgangwalther
Copy link
Contributor

I'd be fine with doing the plv8 check only in the non-JIT test-cases of the postgres test for now to get this out. But I'd like to split this off to make sure we also have reasonable coverage of MemoryDenyWriteExecute being turned on.

Makes sense to me.

An idea I just had was that we could actually build a single VM test for all extensions that require testing using the VM-test framework, but that'd obviously have the downside that postgresqlPackages.X.tests would fail if an unrelated extension is broken.

I thought about that as well, but as you say this won't work nicely for passthru tests.. so don't think this is that helpful overall.

Idk. personally I'd be fine with the amount of VM tests, it's not like I'd expect contributors to run all of them.

I think we can reduce the number of VM tests a bit as well, by moving some of the tests into the extension's derivation as non-VM. I already did that with two of the extensions in the passthru.tests PR. There are two more that could be done like that (tsja and wal2json, IIRC). They just need a bit more fiddling with the code than I was willing to invest in that PR.

@Ma27
Copy link
Member

Ma27 commented Nov 13, 2024

Makes sense to me.

Great. If @mweinelt is fine with that, I'd fix the PR up accordingly tomorrow.

I'll split it into another VM test (and tryo to get the wal2json/tsja into non-VM-tests - unless you're motivated to invest tie into that). Won't happen immediately though.

@mweinelt mweinelt added the backport release-24.11 Backport PR automatically label Nov 15, 2024
@mweinelt @Ma27
nixos/postgresql: rename extraPlugins to extensions
223a6c6 
This is the upstream lingo, and it makes everything slightly less
confusing.
mweinelt and others added 2 commits November 16, 2024 17:29
@mweinelt @Ma27
nixos/postgresql: create infrastructure for relaxing systemd hardening
d370af0 
By matching on the package names of the plugins passed into the package
we can relax the systemd unit hardening as needed.
@mweinelt @jtojnar @Ma27
nixosTests.postgresql: test hardening gets relaxed
e198536 
The plv8 plugin requires access to pkey syscalls. The execution will
crash hard when it is not allowed by the syscall filter.

Co-Authored-By: Jan Tojnar <[email protected]>
@Ma27
nixos/tests/postgresql: test plv8 hardening on non-JIT variants only
68d9643 
PostgreSQL with JIT support enabled doesn't work with plv8. Hence, we'd
get an evaluation failure for each
`nixosTests.postgresql.postgresql.postgresql_jit_X`.

This should be restructured in the future (less VM tests for custom
extensions, but a single VM test for this case to cover). For now, we
should get this fix out and this is a good-enough approach.
@Ma27 Ma27 merged commit 97a911e into NixOS:master Nov 16, 2024
11 of 12 checks passed
@github-actions GitHub Actions
Copy link
Contributor

Successfully created backport PR for release-24.11:

@github-actions github-actions bot mentioned this pull request Nov 16, 2024
Merged
1 task
@mweinelt mweinelt deleted the pg-plugin-relax branch November 16, 2024 21:42
@SuperSandro2000
Copy link
Member

the docs where forgotten to be updated #358159

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 approved these changes

@jtojnar jtojnar jtojnar left review comments

@wolfgangwalther wolfgangwalther wolfgangwalther approved these changes

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 backport release-24.11 Backport PR automatically
Projects
None yet
Milestone
24.11
Development

Successfully merging this pull request may close these issues.
5 participants
@mweinelt @Ma27 @wolfgangwalther @SuperSandro2000 @jtojnar